Skip to content

Conversation

@navin772
Copy link
Member

@navin772 navin772 commented Jun 12, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Adds the timestamp parameter to HistoryUpdatedParams class as per the updated bidi spec - https://w3c.github.io/webdriver-bidi/#event-browsingContext-historyUpdated

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

• Add timestamp parameter to HistoryUpdatedParams class
• Update constructor and JSON parsing method
• Add validation for timestamp parameter
• Align with updated WebDriver BiDi specification


Changes walkthrough 📝

Relevant files
Enhancement
browsing_context.py
Add timestamp parameter to HistoryUpdatedParams                   

py/selenium/webdriver/common/bidi/browsing_context.py

• Added timestamp parameter to HistoryUpdatedParams constructor

Updated from_json method to parse and validate timestamp
• Added
validation ensuring timestamp is non-negative integer
• Updated class
to match WebDriver BiDi specification

+7/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Jun 12, 2025
    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    5678 - Not compliant

    Non-compliant requirements:

    • Fix ChromeDriver connection failure error that occurs after first instance
    • Resolve "Error: ConnectFailure (Connection refused)" for subsequent ChromeDriver instances
    • Ensure proper ChromeDriver instantiation on Ubuntu 16.04.4 with Chrome 65.0.3325.181

    1234 - Not compliant

    Non-compliant requirements:

    • Fix JavaScript execution in link href on click() method
    • Ensure JavaScript alerts are triggered when clicking links with JavaScript in href
    • Restore functionality that worked in version 2.47.1 but broke in 2.48.0/2.48.2
    • Fix compatibility with Firefox 42.0

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing Tests

    The addition of a new required parameter to the constructor and validation logic lacks corresponding unit tests to verify the timestamp validation behavior and proper instantiation.

    def __init__(
        self,
        context: str,
        timestamp: int,
        url: str,
    ):
        self.context = context
        self.timestamp = timestamp
        self.url = url
    
    @classmethod
    def from_json(cls, json: dict) -> "HistoryUpdatedParams":
        """Creates a HistoryUpdatedParams instance from a dictionary.
    
        Parameters:
        -----------
            json: A dictionary containing the history updated parameters.
    
        Returns:
        -------
            HistoryUpdatedParams: A new instance of HistoryUpdatedParams.
        """
        context = json.get("context")
        if context is None or not isinstance(context, str):
            raise ValueError("context is required and must be a string")
    
        timestamp = json.get("timestamp")
        if timestamp is None or not isinstance(timestamp, int) or timestamp < 0:
            raise ValueError("timestamp is required and must be a non-negative integer")
    
        url = json.get("url")
        if url is None or not isinstance(url, str):
            raise ValueError("url is required and must be a string")
    
        return cls(
            context=context,
            timestamp=timestamp,
            url=url,
        )

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 12, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add timestamp validation in constructor

    Add type validation for the timestamp parameter in the constructor to ensure
    consistency with the from_json method. This prevents invalid timestamp values
    from being passed directly to the constructor.

    py/selenium/webdriver/common/bidi/browsing_context.py [335-343]

     def __init__(
         self,
         context: str,
         timestamp: int,
         url: str,
     ):
    +    if not isinstance(timestamp, int) or timestamp < 0:
    +        raise ValueError("timestamp must be a non-negative integer")
         self.context = context
         self.timestamp = timestamp
         self.url = url
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that validation for the timestamp parameter is only performed in the from_json method. Adding the same validation to the __init__ constructor ensures that HistoryUpdatedParams objects are always created in a valid state, regardless of whether they are instantiated directly or through the factory method. This improves the robustness and consistency of the class.

    Medium
    • Update

    @navin772 navin772 requested review from cgoldberg and shbenzer June 12, 2025 13:34
    Copy link
    Member

    @cgoldberg cgoldberg left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM 👍

    @navin772 navin772 merged commit 1fe6d27 into SeleniumHQ:trunk Jun 12, 2025
    2 of 4 checks passed
    @navin772 navin772 deleted the add-timestamp-HistoryUpdatedParams branch June 12, 2025 18:15
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    B-devtools Includes everything BiDi or Chrome DevTools related C-py Python Bindings Review effort 2/5

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants